Skip to content

fix(github): serialise empty PR author instead of omitting it - #1020

Merged
mbevc1 merged 5 commits into
mainfrom
fix/pr-attestation-empty-author
Jul 22, 2026
Merged

fix(github): serialise empty PR author instead of omitting it #1020
mbevc1 merged 5 commits into
mainfrom
fix/pr-attestation-empty-author

Conversation

@vidhu-balad

@vidhu-balad vidhu-balad commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • When a PR creator's GitHub account is deleted, the GraphQL API returns null for the Author node, resulting in author = "" in Go
  • PREvidence.Author has omitempty, so an empty string is omitted from the JSON payload entirely
  • The server's FoundPullRequestV2.author is a required field — missing it causes validation to fail with "input should be a string extra inputs are not permitted"
  • Fix: guard in buildPREvidence substitutes "unknown" when author == "", covering both PREvidenceForCommitV2 and PREvidenceByPRNumber call sites

Test plan

  • TestBuildPREvidence_EmptyAuthorFallsBackToUnknown added and passes
  • All internal/github tests pass (go test ./internal/github/...)
  • go vet ./internal/github/... is clean

🤖 Generated with Claude Code

@claude

claude Bot commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @vidhu-balad's task in 2m 38s —— View job

Code Review

  • Read PR context and prior review threads
  • Static analysis (go build, go vet)
  • Review the fix and tests
  • Post review

Static analysis

Check Result
go build ./... ✅ pass
go vet ./internal/github/... ./internal/types/... ✅ clean
go test ⚠️ couldn't run — test execution isn't in my allowed tools in this run. Build + vet are green; the added test is self-contained and reads correctly.

Assessment — looks good ✅

This is a clean, minimal fix and the review history converged well.

  • types.go — dropping omitempty from Author (json:"author") is exactly the minimal change needed so a deleted-creator PR serialises "author":"" instead of dropping the key and failing the server's required-field validation.
  • buildPREvidence — assigning Author: author verbatim (no "unknown" substitution) is the right call. It keeps Author consistent with the sibling PRApprovals.Username field (also no omitempty, built verbatim from r.Author.Login at github.go:371), so both a deleted creator and a deleted reviewer now serialise an empty string rather than one being silently dropped and the other not.
  • Test now guards the actual fix — TestBuildPREvidence_EmptyAuthorIsPreserved marshals to JSON and asserts require.Contains(string(b), "author":""). This is the key improvement over earlier iterations: the previous require.Equal(t, "", evidence.Author) was trivially true regardless of the struct tag, so re-adding omitempty would have gone undetected. The JSON assertion now fails if omitempty is reintroduced. 👍

Minor / non-blocking notes

  1. Fixture expansion looks like light scope creep. The assertPRGithub_test.go / attestPRGithub_test.go fixtures were fleshed out to the full V2 shape (author, title, commits, etc.). It's harmless and arguably more realistic, but it isn't strictly required by the empty-author fix — worth confirming it's intentional rather than an incidental change, per the repo's thin-slice convention.

  2. Commit history reverses itself. eaebf9b8 adds the "unknown" fallback and 03f7af57 removes it again. Since CLAUDE.md endorses rebasing to squash a slice, consider squashing these two before merge so the final history shows just the empty-string approach. Not blocking.

  3. @ghost convention (from @mbevc1). GitHub attributes deleted-account content to the reserved @ghost user in its own APIs/UI. Sending "" is fine given the server accepts it (per @vidhu-balad's confirmation), so no change needed — just flagging that "@ghost" would be the option if you ever want parity with GitHub's own display.

Nice work strengthening the test in response to review feedback — the JSON-level assertion is the correct way to pin this behaviour.

mbevc1
mbevc1 reviewed Jul 15, 2026
View reviewed changes
Comment thread internal/github/github.go Outdated
@mbevc1

mbevc1 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Another note; same bug class exists for review authors (approvers). In buildPREvidence, approvers are built from r.Author.Login with no guard, and PRApprovals.Username has no omitempty — so a deleted reviewer account produces "username": "" in the payload.

claude[bot]
claude Bot reviewed Jul 15, 2026
View reviewed changes
Comment thread internal/github/build_pr_evidence_test.go
claude[bot]
claude Bot reviewed Jul 15, 2026
View reviewed changes
Comment thread internal/types/types.go
claude[bot]
claude Bot reviewed Jul 16, 2026
View reviewed changes
Comment thread internal/github/build_pr_evidence_test.go
Comment on lines +172 to +173
require.NoError(t, err)
require.Equal(t, "", evidence.Author,

@claude claude Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion still doesn't guard the actual fix (raised in the earlier review, not yet addressed).

The fix is purely the JSON struct-tag change in types.go (dropping omitempty). But buildPREvidence assigns Author: author verbatim (github.go:316), so require.Equal(t, "", evidence.Author, …) is trivially true regardless of the struct tag — it stays green even if someone re-adds omitempty, which would re-introduce the exact bug this PR fixes.

To pin the behavior, assert on the marshaled JSON so the author key is proven present when empty:

require.NoError(t, err)
require.Equal(t, "", evidence.Author,
    "empty PR author login must be preserved")

b, err := json.Marshal(evidence)
require.NoError(t, err)
require.Contains(t, string(b), `"author":""`,
    "empty author must be serialised, not omitted")

(requires adding "encoding/json" to the import block.)

Fix this →

claude[bot]
claude Bot reviewed Jul 16, 2026
View reviewed changes
Comment thread internal/github/build_pr_evidence_test.go
@vidhu-balad vidhu-balad changed the title fix(github): fall back to "unknown" when PR creator account is deleted fix(github): serialise empty PR author instead of omitting it Jul 16, 2026
@mbevc1
mbevc1 enabled auto-merge (squash) July 17, 2026 14:43
vidhu-balad and others added 5 commits July 22, 2026 17:17
@vidhu-balad @claude
fix(github): fall back to "unknown" when PR creator account is deleted
eaebf9b 
When the GitHub account of a PR creator is deleted, the GraphQL API
returns null for the Author node, resulting in an empty login string.
PREvidence.Author has omitempty, so "" is omitted from the JSON payload,
causing the server's FoundPullRequestV2.author required-field validation
to fail with "input should be a string extra inputs are not permitted".

Guard added in buildPREvidence: if author == "", set it to "unknown".
Both call sites (PREvidenceForCommitV2 and PREvidenceByPRNumber) pass
through this function, so both are covered.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vidhu-balad @claude
fix(github): send empty PR author as "" instead of falling back to "u…
03f7af5 
…nknown"

Remove the omitempty tag from PREvidence.Author so an empty string is
serialised in the JSON payload rather than omitted. Drop the "unknown"
fallback in buildPREvidence — the server's FoundPullRequestV2.author
accepts an empty string directly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vidhu-balad @claude
test(github): update fake PR fixtures to full V2 shape
fab00a9 
The minimal {URL, State} fixtures were being accepted by the server as
FoundPullRequestV1. Removing omitempty from PREvidence.Author means
author:"" is now always sent, which FoundPullRequestV1 rejects
(extra="forbid"). Update both fixtures to include all required V2
fields, matching what buildPREvidence actually produces.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vidhu-balad @claude
test(github): add commit entry to fake PR fixtures
ecf2172 
Empty Commits slice is omitted by omitempty, causing FoundPullRequestV2
validation to fail with "commits: Field required". Add a minimal commit
so the slice is non-empty and serialised correctly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vidhu-balad @claude
test(github): assert empty author is serialised in JSON, not omitted
3b2f4da 
The struct-field assertion passes regardless of the omitempty tag.
Adding a JSON marshal check pins the actual wire behaviour: if omitempty
were re-added to PREvidence.Author the test would fail.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vidhu-balad
vidhu-balad force-pushed the fix/pr-attestation-empty-author branch from ec85bb4 to 3b2f4da Compare July 22, 2026 16:17
@mbevc1
mbevc1 merged commit 48e3f72 into main Jul 22, 2026
16 of 17 checks passed
@mbevc1
mbevc1 deleted the fix/pr-attestation-empty-author branch July 22, 2026 16:26
@coderabbitai coderabbitai Bot mentioned this pull request Jul 25, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@claude claude[bot] claude[bot] left review comments
@mbevc1 mbevc1 mbevc1 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vidhu-balad @mbevc1